http_response_code should warn if headers already sent
#10744
Conversation
This would fail silently otherwise. The warning should be similar to the one that header emits (the code is some copy and paste from main/SAPI.c, to match). It'll also return false in that case. Fixes phpGH-10742
There was a problem hiding this comment.
This looks good to me, other than that the failing test needs to be fixed.
diff --git a/sapi/fpm/tests/log-suppress-output.phpt b/sapi/fpm/tests/log-suppress-output.phpt
index 5a5e7bb954..a507180e99 100644
--- a/sapi/fpm/tests/log-suppress-output.phpt
+++ b/sapi/fpm/tests/log-suppress-output.phpt
@@ -38,7 +38,7 @@ function doTestCalls(FPM\Tester &$tester, bool $expectSuppressableEntries)
$tester->request(query: 'test=output', uri: '/ping')->expectBody('pong', 'text/plain');
$tester->expectAccessLog("'GET /ping?test=output' 200", suppressable: false);
- $tester->request(headers: ['X_ERROR' => 1])->expectBody('Not OK');
+ $tester->request(headers: ['X_ERROR' => 1])->expectStatus('500 Internal Server Error')->expectBody('Not OK');
$tester->expectAccessLog("'GET /log-suppress-output.src.php' 500", suppressable: false);
$tester->request()->expectBody('OK');
@@ -54,8 +54,8 @@ function doTestCalls(FPM\Tester &$tester, bool $expectSuppressableEntries)
$src = <<<EOT
<?php
if (isset(\$_SERVER['X_ERROR'])) {
- echo "Not OK";
http_response_code(500);
+ echo "Not OK";
exit;
}
echo \$_REQUEST['test'] ?? "OK";
|
FWIW while it doesn't affect this PR, there are a lot of callers of |
Well, |
|
Obviously not in this PR, but yeah. If it is actually signed, that's a bit confusing because of the function's signature, which is what I was basing it off of. |
|
Which function are you referring to? |
|
Augh, I think I mixed it up with |
|
@NattyNarwhal Ah yes, |
|
I changed it back to int because that's what the API is defined as for now; if that API does get changed to be uint32_t, it'll have to be a separate PR. |
|
Test failure seems spurious, can't reproduce on amd64/arm64 Linux/debug/NTS here? |
I agree, it looks spurious. |
|
Pinging to see if this is still mergeable. |
|
@NattyNarwhal Oh, I thought you have merge access. I can merge it tomorrow. 🙂 |
|
Thank you @NattyNarwhal! |
This would fail silently otherwise. The warning should be similar to the one that header emits (the code is some copy and paste from
main/SAPI.c, to match). It'll also return false in that case.Fixes GH-10742